-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json #40875
Conversation
In 69893f5 I've started to explore implementing this in the editor and quickly ran into an issue with specificity — currently the If we then up the specificity of the block level so that it also includes the block className (e.g. So, I think this means that for this implementation to work, we might need to explore one of the following options:
So, this solution / approach might need to incorporate the some of the ideas from the presets approach in #39708 in order to be viable (it might be a neater approach overall if we can consolidate a bit). I'll continue hacking away at this over the coming days. |
Size Change: +883 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
I've taken a tiny step forward in b2bc04a to experiment with getting the classname for the block and combining that with the container-id classname to increase specificity. If we also remove the fallback styles, then this (at least superficially) seems to solve the specificity issue (on the frontend, at least): 😀 Next up, to see if this is viable, some more to-dos (I'm jotting them down here so that I remember!):
|
I like this idea @andrewserong Thanks for working on it. Just for the record, I tried experimenting with splitting the CSS vars, one for root and another for layout over in #41040 But I think I like the classname/CSS approach better, especially if we can tie it into any meaningful semantic classnames work we do. |
Oh, nice, thanks for digging in! I think the more we experiment the better, that we way can compare concrete examples 😀 |
I think I've figured out how to get this working, hacked in 896c46e, which appears to mean we now have styles rendered out with the correct specificity so that it goes (from highest specificity to least):
Now that that's hacked in there and seems to be working, next week I'll:
|
896c46e
to
e4bdd01
Compare
Still very much a hack, but in 4492cd7, I've now got the approach working in the site editor, so adjusting spacing at the block level in global styles now appears to be working. Here's a not exciting screenshot of the styles output: Here's a slightly more exciting screengrab of adjusting block spacing in global styles for the social icons block: Next up: work out a better place to put everything, and perhaps see if we can consolidate with ideas from #39708. |
); | ||
|
||
if ( isset( static::LAYOUT_STYLES[ $css_property ] ) ) { | ||
$declaration['selectors'] = static::LAYOUT_STYLES[ $css_property ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice trick! I really like how the output CSS works.
I'd be interested to learn whether layout could be abstracted from this class somehow. I'm only mentioning since layout is growing and probably will grow more tendrils in the form of exceptions and further rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! Same here — my goal was to hack the code in wherever I could find to get this working. Now that it seems to mostly be working, the next step is to find a better and more logical home for the code changes 🙂
Let me know if you have any strong ideas on where anything should live!
This is coming along really well so far. Thanks for running with it. I'm feeling positive about the direction, especially if we look into the use of CSS vars and/or classnames to drive the layout styles. Referring to your previous work in #38974 |
784b457
to
46704cf
Compare
Would this PR also resolve #40516? |
Oh, thanks for linking that one @ramonjd! I think so — so far with this exploration, it looks like it should be possible for us to handle all the |
2c23f89
to
2d0ff82
Compare
I've given this a rebase and reworked part of the PHP implementation slightly to move the layout gap styles generation to its own function. This way it plays nicely with the changes from: #41160 so that the gap styles per block are output in the individual style tags for each of those blocks. In the process, I've removed the CSS variable generation in PHP. For backwards compatibility, we'll probably want to add it back in again as a preset, outside of the
|
* Each element is a direct mapping from the CSS property name to the | ||
* path to the value in theme.json & block attributes. | ||
*/ | ||
const PROPERTIES_METADATA = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'wideSize' => null, | ||
), | ||
'spacing' => array( | ||
'blockGap' => null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid settings are added so that blockGap
can be set to null
instead of top
— as of this PR, the blockGap
value will be valid at any level.
There's still lots more that needs to be tidied up, but in dca159d I now have the gap value in global styles in the site editor being generated from the definitions in Top-of-mind things to implement next:
|
This commit merges the remaining changes from [WordPress/gutenberg#40875 Gutenberg PR 40875]. It's Part 2 (see [54162] for Part 1) of a layout improvement initiative and targets `wp_get_layout_style()` in `layout.php`. Context: The overall initiative is to improve layout block support: >to use centralised layout definitions, output base layout styles from global styles, introduce a layout type semantic classname, reduce duplication of container class and style tag output, and fix blockGap at the block level in global styles. Changes include: * Adding an optional parameter `$block_spacing` to `wp_get_layout_style()` for setting a custom spacing on the block. * Adding handle for the block spacing. * Using the style engine to to enqueue and render layout styles via `wp_style_engine_get_stylesheet_from_css_rules()`. * Introduces a new test file for `wp_get_layout_style()`. Follow-up to [54162], [54160], [54159], [53421], [52380], [53085], [52069]. Props andrewserong, isabel_brison, costdev, hellofromTonya. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54274 602fd350-edb4-49c9-b593-d223f7449a82
This commit merges the remaining changes from [WordPress/gutenberg#40875 Gutenberg PR 40875]. It's Part 2 (see [54162] for Part 1) of a layout improvement initiative and targets `wp_get_layout_style()` in `layout.php`. Context: The overall initiative is to improve layout block support: >to use centralised layout definitions, output base layout styles from global styles, introduce a layout type semantic classname, reduce duplication of container class and style tag output, and fix blockGap at the block level in global styles. Changes include: * Adding an optional parameter `$block_spacing` to `wp_get_layout_style()` for setting a custom spacing on the block. * Adding handle for the block spacing. * Using the style engine to to enqueue and render layout styles via `wp_style_engine_get_stylesheet_from_css_rules()`. * Introduces a new test file for `wp_get_layout_style()`. Follow-up to [54162], [54160], [54159], [53421], [52380], [53085], [52069]. Props andrewserong, isabel_brison, costdev, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54274 git-svn-id: http://core.svn.wordpress.org/trunk@53833 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This commit merges the remaining changes from [WordPress/gutenberg#40875 Gutenberg PR 40875]. It's Part 2 (see [54162] for Part 1) of a layout improvement initiative and targets `wp_get_layout_style()` in `layout.php`. Context: The overall initiative is to improve layout block support: >to use centralised layout definitions, output base layout styles from global styles, introduce a layout type semantic classname, reduce duplication of container class and style tag output, and fix blockGap at the block level in global styles. Changes include: * Adding an optional parameter `$block_spacing` to `wp_get_layout_style()` for setting a custom spacing on the block. * Adding handle for the block spacing. * Using the style engine to to enqueue and render layout styles via `wp_style_engine_get_stylesheet_from_css_rules()`. * Introduces a new test file for `wp_get_layout_style()`. Follow-up to [54162], [54160], [54159], [53421], [52380], [53085], [52069]. Props andrewserong, isabel_brison, costdev, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54274 git-svn-id: https://core.svn.wordpress.org/trunk@53833 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@mtias, @justintadlock is already working on bringing visibility to this |
There is no default 'display:flex' in the 6.1 beta 1 WordPress version for a buttons block. As a result - flex justify classes don't work. In the WordPress 6.0: In the 6.1 beta 1: Is this a bug or a new feature? |
@exstheme thanks for reporting the issue you've run into. Is your theme dequeueing the global styles inline CSS |
Hello, dear @andrewserong ! Thank you very much for your answer and your time! And thank you for your pointing me to
because we're not using them in our theme. I have found that 14kB of inline styles that are printed in the head and cannot be cached as a normal CSS file is a little bit overkilling. Maybe it would be better to add this super important styles to the
And why they are printed separately? The selector is the same. For now I will add these CSS rules to my theme's CSS file to be compatible with WP 6.1. Once again thank you for your attention! Best regards! |
I noticed this breaking change if the theme tries to override the CSS variable on the block level. I'm experimenting with re-building my site with Beaumont and this CSS rule no longer works - https://themes.trac.wordpress.org/browser/beaumont/0.1.5/style.css#L352. I just wanted to mention this issue. But I think my example here can be solved by adding |
Thanks for the update @exstheme, glad you could find a workaround. If you're looking into adopting
Most of the styles are grouped together, but the |
Thanks for the heads-up @Mamaduka! I believe block-level gap control wasn't previously available in global styles, so yes, it does sound like for some themes there'll be a breaking change if folks have been overriding the CSS variable like that. The Post Template block is a bit of a weird case because it currently partially opts-in to the layout support, and it also has its own existing Do you know what the desired result is from that existing rule? (If we have a concrete use case, we can add a suggested fix for themes like this in the dev note if it's a common thing in themes — they might be able to target the desired CSS properties now instead of relying on the CSS variable?). It might also be worth opening up a separate issue for the problem of overriding post template spacing so we can investigate further, if it isn't already covered in the request in #39380 |
Sounds good. I'll have a look at #39380.
I think mentioning a simple upgrade path for similar cases in the DevNote would be great. Cc. @andersnoren, any thoughts on desired results here? |
@Mamaduka Beaumont has separators between each item in the Query Loop, and the This is how it looks with the CSS variable set to 0 in style.css (running 6.0 without Gutenberg): And without the CSS variable set to 0 (also how it looks running Gutenberg right now): |
Awesome, thank you for confirming George and Anders, I'll download Beaumont and have a play to see what the best fix might be. |
It looks like you just recently added a styling rule to account for this one @andersnoren, but from testing locally, I think you might also be able to add the following to
Here's how adding that looks in my local environment: I'd be slightly hesitant about suggesting that as a general fix, though, because the Post Template block doesn't properly support switching between flow and flex layouts — switching to grid adds the block's ad hoc
Good to know. If we encounter any other cases of this, I'm happy to write something up for the dev notes. If not, it might be better to leave it off since it appears to be quite a specific case particular to that block. As an aside, that's a very nice looking theme! |
@andrewserong Oh, I managed to completely miss that – I must have had the Gutenberg plugin inactive when I tested it. I'm always happy to remove a little bit of CSS, so thanks for the tip (and the kind words)! |
This change backports the following changes from Gutenberg repository: - [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087 - [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792 - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665 - [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085 Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`. Props andrewserong, aaronrobertshaw, isabel_brison. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54159 602fd350-edb4-49c9-b593-d223f7449a82
This commit merges the remaining changes from [WordPress/gutenberg#40875 Gutenberg PR 40875]. It's Part 2 (see [54162] for Part 1) of a layout improvement initiative and targets `wp_get_layout_style()` in `layout.php`. Context: The overall initiative is to improve layout block support: >to use centralised layout definitions, output base layout styles from global styles, introduce a layout type semantic classname, reduce duplication of container class and style tag output, and fix blockGap at the block level in global styles. Changes include: * Adding an optional parameter `$block_spacing` to `wp_get_layout_style()` for setting a custom spacing on the block. * Adding handle for the block spacing. * Using the style engine to to enqueue and render layout styles via `wp_style_engine_get_stylesheet_from_css_rules()`. * Introduces a new test file for `wp_get_layout_style()`. Follow-up to [54162], [54160], [54159], [53421], [52380], [53085], [52069]. Props andrewserong, isabel_brison, costdev, hellofromTonya. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54274 602fd350-edb4-49c9-b593-d223f7449a82
return <layoutType.save layout={ layout } { ...props } />; | ||
if ( css ) { | ||
return <style>{ css }</style>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is old, but I'm not sure I understand why we have a css
prop in this component, why not just use <style>{css}</style>
instead of <LayoutStyle css={css} />
? am I missing anything? It seems it has nothing to do with layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could totally be refactored. If I remember correctly, the thinking at the time was to have a single entry point for outputting layout styles via the LayoutStyle
component, as it includes a check that the layout type is valid and made for a little more consistency in the consuming code... the thinking at the time was also that we might further refactor the innards of LayoutStyle
to not necessarily output a style
tag, so in theory, having that abstracted into the LayoutStyle
component could be beneficial.
There's been a fair few changes since this PR was merged, though, and with the recent work on #52888 and I see #54466 has just landed to refactor some of the layout style output, I don't think we need to hold onto any of the decisions in this older PR 🙂
What?
Refactor the Layout block support to use centralised layout definitions, output base layout styles from global styles, introduce a layout type semantic classname, reduce duplication of container class and style tag output, and fix blockGap at the block level in global styles.
Note: This is intentionally a complex PR that attempts to make many changes to the Layout support all at once. Unfortunately, for the changes proposed, I don't think there's an easy way to split it out into smaller changes, as it rethinks how the Layout block support works internally, by shifting a fair bit of logic to global styles.
Fixes #39789, part of #39336, and is a step forward toward addressing duplicate layout styles as in #41434 and using semantic classnames to link Layout styles as raised in #38719. This PR explores an idea from #39870 (comment) as a (potential) alternative to #39870:
theme.json
, used for generating styles in both the editor and on the site front-endis-layout-flow
andis-layout-flex
)blockGap
values at the block level withintheme.json
The original goal of this PR is to see if we can support setting
blockGap
at the block level intheme.json
and output styles that correctly target blocks that opt-in to the layout block support. While we're at it, we can progress a few different ideas we'd like to address in the layout support:wp-container-$id
classnames and style tags — in this PR we start to do this by outputting base styles at the root.is-layout-flow
and.is-layout-flex
rules, so that the individual layout rules that we generate are just for those blocks that deviate from the base styling / settings for each layout type.Why?
The problem this PR initially set out to solve was to allow defining
blockGap
values at the block level withintheme.json
. However, since this involves a decently sized change to how the layout support works, it's a good opportunity to re-think a few foundational things about how the layout support works, and progress some of those areas forward to address some long-standing issues.The overall goal is to reduce duplication, consolidate how things work, and set us up for follow-up PRs where we can add in remaining semantic classnames, and hopefully make it easier to add additional layout types in the future.
How?
.is-layout-flow
,.is-layout-flex
)wp-container-$id
classname andstyle
tag if the block contains non-default Layout values (this results in fewerstyle
tags being output)Testing Instructions
Update the
theme.json
for your theme to specify ablockGap
value for a particular block. E.g.On the front end of your site, the block-level styles should take effect.
Open up the site editor, and within global styles, go to update the block-level block spacing for blocks like Buttons and Social Icons. It should work as expected.
Check that tests pass, e.g.:
Examples to test that the Layout support functions as expected
settings.spacing.blockGap
totrue
intheme.json
, and in global styles in the site editor, you should be able to now set a gap value for blocks such as Social Icons or Columns.settings.spacing.blockGap
tonull
intheme.json
, and ensure that the defaultflex
gap styles for blocks like Social Icons and Columns works as expected.flex
gap styles for blocks like Social Icons and Columns should work as expected (e.g.0.5em
default gap for Social Icons,2em
default gap for Columns).Screenshots or screencast
style
tags for blocks that only use default settings)style
tags have been removed — there is still more to go, to be addressed in follow-ups)